Skip to content

Fixes: FallBack to LoadDataset, Dup Arguments, Savetensor Saving - #27

Merged
garrett361 merged 7 commits into
mainfrom
fix/dataset
Jul 8, 2025
Merged

garrett361 merged 7 commits into
mainfrom
fix/dataset

Conversation

@fabianlim

@fabianlim fabianlim commented Jul 4, 2025 •

Copy link
Copy Markdown
Collaborator

Companion PR to #6, and some additional fixes

  • datasets.load_dataset does a schema check and sometimes may fail on some files. This fallsback to pandas
  • remove duplicated arguments
  • make savetensor saving as default

API CHANGE:
if the dataset has tools or documents columns in the dataset, add this to the arguments

Signed-off-by: Yu Chin Fabian Lim <flim@sg.ibm.com>
fabianlim added 3 commits July 4, 2025 18:30
Signed-off-by: Yu Chin Fabian Lim <flim@sg.ibm.com>
Signed-off-by: Yu Chin Fabian Lim <flim@sg.ibm.com>
Signed-off-by: Yu Chin Fabian Lim <flim@sg.ibm.com>
@fabianlim

Copy link
Copy Markdown
Collaborator Author

@garrett361 TestPaddingFree is failing because I think the runners do not have cuda

def test_padding_free(self, model_name: str) -> None:
        torch.manual_seed(42)
>       model, cfg = self.get_fa2_model_and_cfg(model_name)

@garrett361

Copy link
Copy Markdown
Owner

Yeah expected, I should add a skip if CUDA isn't available

@fabianlim

Copy link
Copy Markdown
Collaborator Author

@garrett361 it seems they now run all tests.. previously they only run one test

Previously it was like this #25

Run pytest open_instruct/test_utils.py
============================= test session starts ==============================
platform linux -- Python 3.10.10, pytest-8.4.1, pluggy-1.6.0
rootdir: /home/runner/work/open-instruct/open-instruct
configfile: pyproject.toml
collected [7](https://github.com/garrett361/open-instruct/actions/runs/16009216897/job/45162936130#step:5:8) items

open_instruct/test_utils.py .......                                      [100%]

============================== 7 passed in 14.[8](https://github.com/garrett361/open-instruct/actions/runs/16009216897/job/45162936130#step:5:9)5s ==============================

@garrett361

Copy link
Copy Markdown
Owner

Thought I saw that as well

@Swanand-Kadhe

Copy link
Copy Markdown
Collaborator

It is not necessary to include tools documents in dataset_target_columns

If we look at the transformation function, first tokenization and masking happens, and then columns are removed. tools and documents columns are necessary for correctly applying the chat template (and thereby for tokenization and masking). However, after applying chat template and tokenization, the only columns needed are input_ids, labels, and attention_mask.

Of course, including tools and documents in dataset_target_columns does not impact the correctness, it will just increase the size of cached data.

Also, tools and documents are going to be ignored in default o-i sft_tulu_tokenize_and_truncate_v1. In porting changes to main, we do not have ported additional_inputs in sft_tulu_tokenize_and_truncate_v1.

@fabianlim fabianlim changed the title Add FallBack to LoadDataset Fixes: FallBack to LoadDataset, Dup Arguments, Savetensor Saving Jul 8, 2025
@fabianlim

Copy link
Copy Markdown
Collaborator Author

@garrett361 can we merge this soon? we just ignore the failing test?

Comment thread open_instruct/dataset_transformation.py Outdated
from open_instruct.utils import hf_whoami

# MOVE THIS SOMEWHERE
from datasets import load_dataset as lds

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: move the lds import inside load_dataset so it's not exposed?

Comment thread open_instruct/finetune.py Resolved
Comment thread open_instruct/finetune.py Resolved
Comment thread open_instruct/model_utils.py Resolved

@garrett361 garrett361 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small questions. Can merge after they're resolved.

Comment thread open_instruct/dataset_transformation.py Outdated
dfs.append(reader(file, orient='records'))

raw_datasets = Dataset.from_pandas(pd.concat(dfs))
del dfs

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: don't need to del dfs since it goes out of scope immediately, right?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well yes.. was being paranoid here.

Signed-off-by: Yu Chin Fabian Lim <flim@sg.ibm.com>
@garrett361
garrett361 merged commit 90a8807 into main Jul 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants